-
Notifications
You must be signed in to change notification settings - Fork 44
Update RefIccMAX CMake configuration file #473
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request modernizes the RefIccMAX CMake configuration file by transforming it from a basic find module into a comprehensive CMake package configuration file. The changes enable better integration with modern CMake projects and vcpkg package management.
Key changes:
- Introduces modern IMPORTED targets (IccProfLib2, IccXML2, and their static variants) following current CMake best practices
- Adds comprehensive documentation and usage examples for both modern targets and legacy variables
- Implements proper library detection for both shared and static library variants, including debug builds
- Maintains full backward compatibility with existing projects using legacy variables (REFICCMAX_LIBRARIES, REFICCMAX_INCLUDE_DIRS)
| IMPORTED_LOCATION_DEBUG "${ICCXML2_LIBRARY_DEBUG}" | ||
| ) | ||
| endif() | ||
| if(TARGET IccProfLib2) | ||
| set_target_properties(IccXML2 PROPERTIES | ||
| INTERFACE_LINK_LIBRARIES IccProfLib2 | ||
| ) |
Copilot
AI
Jan 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The IccXML2 library's INTERFACE_LINK_LIBRARIES is set to IccProfLib2 (line 166), but there's no check whether IccProfLib2 was found as a shared or static library. If IccProfLib2 is static and IccXML2 is shared, this could cause linking issues. Consider linking to the appropriate variant (IccProfLib2-static if IccXML2 is static, IccProfLib2 if shared) based on which library type was found.
| IMPORTED_LOCATION_DEBUG "${ICCXML2_LIBRARY_DEBUG}" | |
| ) | |
| endif() | |
| if(TARGET IccProfLib2) | |
| set_target_properties(IccXML2 PROPERTIES | |
| INTERFACE_LINK_LIBRARIES IccProfLib2 | |
| ) | |
| IMPORTED_LOCATION_DEBUG "${ICC2XML_LIBRARY_DEBUG}" | |
| ) | |
| endif() | |
| if(TARGET IccProfLib2) | |
| # Prefer linking against shared IccProfLib2 when available | |
| set_target_properties(IccXML2 PROPERTIES | |
| INTERFACE_LINK_LIBRARIES IccProfLib2 | |
| ) | |
| elseif(TARGET IccProfLib2-static) | |
| # Fallback: link against static IccProfLib2-static if only static is available | |
| set_target_properties(IccXML2 PROPERTIES | |
| INTERFACE_LINK_LIBRARIES IccProfLib2-static | |
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential True Positive. Testing in Progress.
|
|
||
| # IccProfLib2 shared library | ||
| if(ICCPROFLIB2_LIBRARY AND NOT TARGET IccProfLib2) | ||
| add_library(IccProfLib2 SHARED IMPORTED) |
Copilot
AI
Jan 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ICCPROFLIB2_LIBRARY is marked as SHARED IMPORTED (line 125), but there's no verification that the library is actually shared. The find_library command finds any library with the given name regardless of type. If a static library is found instead, this will cause a mismatch. Consider using UNKNOWN IMPORTED instead of SHARED IMPORTED and setting the IMPORTED_LINK_INTERFACE_LIBRARIES property appropriately, or verify the library type before creating the target.
| add_library(IccProfLib2 SHARED IMPORTED) | |
| add_library(IccProfLib2 UNKNOWN IMPORTED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential True Positive. Testing in Progress.
|
|
||
| # IccXML2 shared library | ||
| if(ICCXML2_LIBRARY AND NOT TARGET IccXML2) | ||
| add_library(IccXML2 SHARED IMPORTED) |
Copilot
AI
Jan 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The IccXML2 library is marked as SHARED IMPORTED (line 154), but there's no verification that the library is actually shared. The find_library command finds any library with the given name regardless of type. If a static library is found instead, this will cause a mismatch. Consider using UNKNOWN IMPORTED instead of SHARED IMPORTED, or verify the library type before creating the target.
| add_library(IccXML2 SHARED IMPORTED) | |
| add_library(IccXML2 UNKNOWN IMPORTED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential True Positive. Testing in Progress.
| # Fallback for system-wide installation | ||
| find_path(REFICCMAX_INCLUDE_DIR | ||
| NAMES @TARGET_LIB_ICCPROFLIB@/IccProfLibVer.h | ||
| PATHS @CMAKE_INSTALL_FULL_INCLUDEDIR@/@TARGET_INCLUDE_FOLDER@ |
Copilot
AI
Jan 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fallback find_path for REFICCMAX_INCLUDE_DIR (lines 54-57) will search system-wide paths, but it may find an incompatible version if RefIccMAX is installed system-wide. This could cause version mismatches. Consider adding a VERSION check or making the NO_DEFAULT_PATH behavior stricter to avoid mixing installation locations.
| # Fallback for system-wide installation | |
| find_path(REFICCMAX_INCLUDE_DIR | |
| NAMES @TARGET_LIB_ICCPROFLIB@/IccProfLibVer.h | |
| PATHS @CMAKE_INSTALL_FULL_INCLUDEDIR@/@TARGET_INCLUDE_FOLDER@ | |
| # Fallback for system-wide installation (restricted to explicit paths) | |
| find_path(REFICCMAX_INCLUDE_DIR | |
| NAMES @TARGET_LIB_ICCPROFLIB@/IccProfLibVer.h | |
| PATHS @CMAKE_INSTALL_FULL_INCLUDEDIR@/@TARGET_INCLUDE_FOLDER@ | |
| NO_DEFAULT_PATH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential True Positive. Testing in Progress.
| find_package_handle_standard_args(RefIccMAX | ||
| REQUIRED_VARS REFICCMAX_INCLUDE_DIR | ||
| VERSION_VAR REFICCMAX_VERSION | ||
| HANDLE_COMPONENTS |
Copilot
AI
Jan 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The HANDLE_COMPONENTS option is specified in find_package_handle_standard_args (line 106), but there's no code to actually handle components. This will cause CMake to fail if users specify components like find_package(RefIccMAX COMPONENTS IccProfLib2). Either remove HANDLE_COMPONENTS or implement proper component handling by checking RefIccMAX_FIND_COMPONENTS and setting component-specific FOUND variables.
| HANDLE_COMPONENTS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential True Positive. Testing in Progress.
23e5549 to
42afd45
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
StatusModify │ 301 * Function: GetInstance() │ and.... Class Mods... 97 * Purpose: The Reflectance Observer Element │ |
StatusConsidering the issues present, the Libs install across all platforms as expected. The Tools are not ready.
|
vcpkg compile errorsSample iccDumpProfile.cpp.obj : error LNK2001: unresolved external symbol "char const * const icMsgValidateWarning" (?icMsgValidateWarning@@3PEBDEB) |
StatusWed Jan 14 01:09:23 AM UTC 2026 Commit Info
5c02515 Windows ReproductionWindows vcpkg install[core,tools]Unix ReproductionUnix vcpkg install
Expected Output |
PR Author Commentvcpkg install across Windows, Linux & macOS is not 100% installable for core & tools due to vcpkg dependency issues. Path of least resistance is to move forward with VCPKG PR for Windows-only Cmake Target. |
StatusWed Jan 14 02:57:36 AM UTC 2026 For 'vcpkg install iccdev' the Law of Diminishing Returns has been met. The Cmake alignment will be Committed, the vcpkg configurations withdrawn. vcpkg is a nice to have thing, but not necessary and is a Time Sink! |
5c02515 to
b494bff
Compare
Update RefIccMAX CMake configuration file
b494bff to
5be4e0d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| if(TARGET IccProfLib2) | ||
| set_target_properties(IccXML2 PROPERTIES | ||
| INTERFACE_LINK_LIBRARIES IccProfLib2 | ||
| ) | ||
| endif() |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The INTERFACE_LINK_LIBRARIES is only set if the IccProfLib2 target exists, but IccXML2 likely depends on IccProfLib2 in all cases. If IccProfLib2 doesn't exist but IccProfLib2-static does (and was aliased to IccProfLib2), this dependency won't be properly set. Consider checking for either IccProfLib2 or IccProfLib2-static, or rely on the alias being created.
| NAMES IccXML2 | ||
| HINTS "${REFICCMAX_PREFIX}/lib" | ||
| PATHS @CMAKE_INSTALL_FULL_LIBDIR@ | ||
| NO_DEFAULT_PATH | ||
| ) | ||
| MARK_AS_ADVANCED(REFICCMAX_INCLUDE_DIR) | ||
|
|
||
| find_library(ICCXML2_LIBRARY_DEBUG | ||
| NAMES IccXML2d | ||
| HINTS "${REFICCMAX_PREFIX}/debug/lib" | ||
| NO_DEFAULT_PATH | ||
| ) | ||
|
|
||
| # Set path to the library. | ||
| FIND_LIBRARY( REFICCMAX_LIBRARIES | ||
| NAMES @TARGET_LIB_ICCPROFLIB@ | ||
| PATHS @CMAKE_INSTALL_FULL_LIBDIR@/ | ||
| find_library(ICCXML2_STATIC_LIBRARY | ||
| NAMES IccXML2-static |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The library name "IccXML2" is hardcoded on lines 82, 89, and 95, while the IccProfLib2 searches use the configurable @TARGET_LIB_ICCPROFLIB@ variable (line 62, 69, 75). For consistency and maintainability, consider using a @TARGET_LIB_ICCXML@ variable instead of hardcoding the library name.
| # Legacy support - set REFICCMAX_LIBRARIES if not already set | ||
| if(NOT REFICCMAX_LIBRARIES) | ||
| if(ICCPROFLIB2_LIBRARY) | ||
| list(APPEND REFICCMAX_LIBRARIES "${ICCPROFLIB2_LIBRARY}") | ||
| elseif(ICCPROFLIB2_STATIC_LIBRARY) | ||
| list(APPEND REFICCMAX_LIBRARIES "${ICCPROFLIB2_STATIC_LIBRARY}") | ||
| endif() | ||
| endif() | ||
|
|
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an inconsistency in REFICCMAX_LIBRARIES values. Earlier sections (lines 135, 148, 169, 187) append CMake target names (e.g., IccProfLib2, IccXML2), while the legacy support section appends raw library paths (ICCPROFLIB2_LIBRARY, ICCPROFLIB2_STATIC_LIBRARY). This mixing of target names and paths in the same variable will confuse consumers. The legacy section should either be removed (since targets are set above) or the logic should be restructured to use only paths for legacy support.
| # Legacy support - set REFICCMAX_LIBRARIES if not already set | |
| if(NOT REFICCMAX_LIBRARIES) | |
| if(ICCPROFLIB2_LIBRARY) | |
| list(APPEND REFICCMAX_LIBRARIES "${ICCPROFLIB2_LIBRARY}") | |
| elseif(ICCPROFLIB2_STATIC_LIBRARY) | |
| list(APPEND REFICCMAX_LIBRARIES "${ICCPROFLIB2_STATIC_LIBRARY}") | |
| endif() | |
| endif() |
| find_library(ICCXML2_LIBRARY | ||
| NAMES IccXML2 | ||
| HINTS "${REFICCMAX_PREFIX}/lib" | ||
| PATHS @CMAKE_INSTALL_FULL_LIBDIR@ | ||
| NO_DEFAULT_PATH | ||
| ) | ||
| MARK_AS_ADVANCED(REFICCMAX_INCLUDE_DIR) | ||
|
|
||
| find_library(ICCXML2_LIBRARY_DEBUG | ||
| NAMES IccXML2d | ||
| HINTS "${REFICCMAX_PREFIX}/debug/lib" | ||
| NO_DEFAULT_PATH | ||
| ) | ||
|
|
||
| # Set path to the library. | ||
| FIND_LIBRARY( REFICCMAX_LIBRARIES | ||
| NAMES @TARGET_LIB_ICCPROFLIB@ | ||
| PATHS @CMAKE_INSTALL_FULL_LIBDIR@/ | ||
| find_library(ICCXML2_STATIC_LIBRARY | ||
| NAMES IccXML2-static | ||
| HINTS "${REFICCMAX_PREFIX}/lib" | ||
| PATHS @CMAKE_INSTALL_FULL_LIBDIR@ | ||
| NO_DEFAULT_PATH | ||
| ) |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The IccXML2 library searches (lines 81-99) are missing the PATHS argument with CMAKE_INSTALL_FULL_LIBDIR that the IccProfLib2 searches have (lines 64, 77). This inconsistency means IccXML2 libraries won't be found if the package is installed in a system-wide location but the REFICCMAX_PREFIX calculation doesn't work. Add the PATHS argument for consistency and robustness.
| set(REFICCMAX_INCLUDE_DIRS "${REFICCMAX_INCLUDE_DIR}") | ||
|
|
||
| # Set up include directories for all subdirectories | ||
| # IccXML2 headers need access to IccProfLib2 headers via relative includes |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment states "IccXML2 headers need access to IccProfLib2 headers via relative includes" but the actual includes in IccXML files use direct includes like #include "IccProfile.h" without relative paths. The comment should be updated to accurately reflect that IccXML2 headers include IccProfLib2 headers directly, requiring both directories to be in the include path.
| # IccXML2 headers need access to IccProfLib2 headers via relative includes | |
| # IccXML2 headers directly include IccProfLib2 headers, so both dirs must be in the include path |
|
|
||
| # IccProfLib2 shared library | ||
| if(ICCPROFLIB2_LIBRARY AND NOT TARGET IccProfLib2) | ||
| add_library(IccProfLib2 SHARED IMPORTED) |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The library type is hardcoded as SHARED, but the actual library type should be determined from the library file extension or build configuration. If only static libraries were built, this will create an incorrectly typed IMPORTED target. Consider using UNKNOWN library type or checking the actual library type.
| add_library(IccProfLib2 SHARED IMPORTED) | |
| add_library(IccProfLib2 UNKNOWN IMPORTED) |
| find_package_handle_standard_args(RefIccMAX | ||
| REQUIRED_VARS REFICCMAX_INCLUDE_DIR | ||
| VERSION_VAR REFICCMAX_VERSION | ||
| HANDLE_COMPONENTS |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The HANDLE_COMPONENTS flag is specified but no components are actually defined or validated. Either remove the HANDLE_COMPONENTS flag if components are not needed, or implement proper component handling by checking for requested components (e.g., IccProfLib2, IccXML2) in the find_package call.
| HANDLE_COMPONENTS |
Pull Request Checklist